feat: TraceIdRatioBasedSampler and re-sync cross agent tests
#3501
+874
−136
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3501 +/- ##
==========================================
- Coverage 97.77% 97.64% -0.13%
==========================================
Files 426 427 +1
Lines 55918 56012 +94
Branches 1 1
==========================================
+ Hits 54672 54695 +23
- Misses 1246 1317 +71
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
10 times, most recently
from
531c86e to
0356e15
Compare
amychisholm03
changed the title
TraceIdRatioBasedSampler and other sampling fixes
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
2 times, most recently
from
88dfa7a to
f2b2208
Compare
bizob2828
requested changes
Nov 18, 2025
Member
bizob2828
left a comment
bizob2828
left a comment
There was a problem hiding this comment.
I haven't fully reviewed but I have some concerns with our approach, especially when you layer on partial granularity/full granularity tracing
| applySamplingDecision(transaction) { | ||
| if (!transaction) return | ||
| // eslint-disable-next-line sonarjs/pseudo-random | ||
| const initPriority = Math.random() |
Member
There was a problem hiding this comment.
This is the sample logic as the adaptive, it's just that it calls its shouldSample method. Feels like this should be in the default Sampler class
Contributor
Author
There was a problem hiding this comment.
I agree, that was my first thought too. However the parameters passed into shouldSample differ, so there's no real repeated logic except for the priority calc and truncation.
bizob2828
reviewed
Nov 18, 2025
| } | ||
|
|
||
| // Is it TraceIdRatioBased? | ||
| const ratioBasedSampler = samplerDefinition?.trace_id_ratio_based |
Member
There was a problem hiding this comment.
this logic should live in the config. we shouldn't have to check if it's a valid ratio here.
Contributor
Author
There was a problem hiding this comment.
I agree that we shouldn't but it's there as kind of a fail safe
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
2 times, most recently
from
0942856 to
c98bd79
Compare
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
2 times, most recently
from
63f0379 to
4bb1061
Compare
bizob2828
reviewed
Nov 19, 2025
bizob2828
reviewed
Nov 19, 2025
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
from
04c45d1 to
5ed77ee
Compare
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
from
5ed77ee to
5c0285f
Compare
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
from
5c0285f to
3dc878c
Compare
amychisholm03
changed the title
TraceIdRatioBasedSampler and other sampling fixesTraceIdRatioBasedSampler and re-sync cross agent tests
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
from
ee10b81 to
d41756c
Compare
amychisholm03
force-pushed
the
NR-3387/ratio-based-sampler
branch
from
8e32efe to
e687350
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR builds off the new sampling architecture defined by #3527.
This PR adds another
Samplerthat is akin to OpenTelemetry'sTraceIdRatioBasedSampler.How to Test
Cross-agent tests are re-synced! The new tests uncovered some missing pieces with our sampling logic, outside of ratio-based sampling, which this PR solves.
Related Issues
Closes #3387 and closes #3480